-
Notifications
You must be signed in to change notification settings - Fork 43
Enable EnKF-only for atmosphere #2010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
… hard-coded (need to revisit)
This PR works along with the following three dependent PRs to enable the EnKF-only configuration for the atmosphere within the global workflow (see detailed description NOAA-EMC/global-workflow#4345) Dependencies: -NOAA-EMC/global-workflow#4345 -NOAA-EMC/GDASApp#2010 -NOAA-EMC/jcb-gdas#219 Resolve - NOAA-EMC/global-workflow#4339 --------- Co-authored-by: Cory Martin <cory.r.martin@noaa.gov>
|
@CoryMartin-NOAA I updated this branch with develop. It now has issues of reading the obs distribution and localization blocks in atm_ens_obs_dist_localizations.yaml.j2 |
|
@bhuang95 I ran into this same problem on Thursday. I don't immediately know what new change broke this, but I think the fix is this: NOAA-EMC/wxflow#65 |
This PR works along with the following three dependent PRs to enable the EnKF-only configuration for the atmosphere within the global workflow (see detailed description NOAA-EMC/global-workflow#4345) Dependencies: -NOAA-EMC/global-workflow#4345 -NOAA-EMC/GDASApp#2010 -NOAA-EMC/jcb#32 Resolve - NOAA-EMC/global-workflow#4339 --------- Co-authored-by: Cory Martin <cory.r.martin@noaa.gov>
This PR works along with the following three dependent PRs to enable the EnKF-only configuration for the atmosphere within the global workflow (see detailed description NOAA-EMC/global-workflow#4345) Dependencies: -NOAA-EMC/global-workflow#4345 -#2010 -NOAA-EMC/jcb#32 Resolve - NOAA-EMC/global-workflow#4339 --------- Co-authored-by: Cory Martin <cory.r.martin@noaa.gov>
|
@bhuang95 does any change here depend on global-workflow or is it the other way around? Meaning, can we (after you or I resolve conflicts) work towards getting this merged in ASAP? |
@CoryMartin-NOAA These two lines with default value: false below should remove the dependency of this PR on the global workflow, and the other way around. GDASApp/parm/atm/atm_ens_config.yaml.j2 Line 78 in 9b75e70
GDASApp/parm/atm/jcb-base.yaml.j2 Line 132 in 9b75e70
The other major changes about obs distribution and localization will be handled in the NOAA-EMC/jcb#32 (merged) and use I addressed the conflicts. I think it should be ready to merge if it looks good to you. |
CoryMartin-NOAA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @bhuang95 I'm going to test this and then approve if it works well
| # Observation things | ||
| # ------------------ | ||
| {% include OBS_LIST_YAML %} | ||
| {% include OBS_DIST_LOCALIZATIONS_YAML %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem that this requires global-workflow to be updated/synchronized, because otherwise this variable is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CoryMartin-NOAA That's a bummer. Thinking about two options,
Option 1:
- Remove Line 20 and add a TODO comment below Line 14
- Change Line 3 to app_path_observations: {{PARMgfs}}/gdas/jcb-gdas/observations/atmosphere-lgetkf
Option 2:
- Add this line below in job-base.yaml.j2 and replace OBS_DIST_LOCALIZATIONS_YAML with "obs_dist_localizations"
- obs_dist_localizations: {{ OBS_DIST_LOCALIZATIONS_YAML | default("{{PARMgfs}}/gdas/atm/atm_ens_obs_dist_localizations.yaml.j2", true) }}
Would any of these work? I am also fine to wait for global-workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Option 2 if it works, because that means we can get rid of the observations/atmosphere-lgetkf directory sooner rather than later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they will be more static than the list of obs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard my deleted comments. I concur with Cory that these parameter should be in jcb-base.yaml.j2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CoryMartin-NOAA @DavidNew-NOAA Looks like we can simply add {% include 'atm_ens_obs_dist_localizations.yaml.j2' %} in the LGETKF yaml. This change passed my JEDI LGETKF run on Ursa.
CoryMartin-NOAA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but we need to not merge until the workflow PR is ready or else things will be broken
This PR works along with the following three dependent PRs to enable the EnKF-only configuration for the atmosphere within the global workflow (see detailed description NOAA-EMC/global-workflow#4345)
Dependencies:
-NOAA-EMC/global-workflow#4345
-NOAA-EMC/jcb-gdas#219
-NOAA-EMC/jcb#32
Resolve